feat: add matrix tests to containers#637
Conversation
CI Test ResultsRun: #29252241893 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-07-13 13:38:05 UTC |
4fb3c21 to
d10ab7f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53fb055c24
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Reliability & Chaos Results❌ 1 failure(s) detected Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/124258703 ❌ chaos: profiler tcmalloc amd64 21 0 3 temXchaos |
rkennke
left a comment
There was a problem hiding this comment.
Review
Scope: 3 files, +739/-25 — README.md, utils/README.md, utils/run-containers-tests.sh. No CI workflow files touched; this only extends the local dev/test script that mirrors CI's container test matrix (glibc/musl × JDK × arch × config), adding --matrix, --run, --fail-fast, and multi-value (all/comma-list) dimension filters, plus a status table and markdown/JSON summary reports.
Overview
The script gains dimension expansion (expand_dimension, expand_jdk_dimension), a skip_reason() table that mirrors known CI exclusions (musl+j9/graal, graal+asan, glibc/aarch64 8-11, musl+sanitizers), a matrix preview/confirmation flow, sequential matrix execution with --fail-fast, and report writers. Docs in both READMEs are updated consistently with the new flags.
Bug found (verified by running the script)
utils/run-containers-tests.sh:907:
if (( MATRIX_TOTAL > 1 || MATRIX_RUNNABLE > 1 || MATRIX_MODE )); thenMATRIX_MODE holds the string "true"/"false", not 1/0. In a bash arithmetic context, a variable holding a non-numeric string is itself re-evaluated as a variable name — since there's no shell variable named true, it silently resolves to 0. So MATRIX_MODE never actually contributes to this condition.
Confirmed with:
./run-containers-tests.sh --matrix --libc=glibc --jdk=21 --arch=x64 --config=debug --container=docker
# → "Error: container runtime 'docker' not found" — i.e. it fell through to the single-cell
# execution path instead of showing the matrix preview/confirmation, even with --run.
Impact is narrow — bare --matrix or partial filters expand to >1 cell and work correctly (verified separately). But it's a real correctness bug in exactly the "preview a specific single-cell matrix explicitly" path the flag is meant to support, and "boolean flag in arithmetic context" is a bug class worth fixing before it recurs elsewhere. Fix:
if (( MATRIX_TOTAL > 1 || MATRIX_RUNNABLE > 1 )) || $MATRIX_MODE; thenMinor / non-blocking
matrix_command()(run-containers-tests.sh:336) is defined but never called anywhere in the script — dead code, presumably superseded by the inline command-building inrun_matrix(). Worth deleting.- Heavy use of
evalfor indirect array access (append_unique_line,read_lines_into_array,read_string_into_array) works but is easy to get wrong later; a bash-4+local -nnameref would be safer if the minimum bash version allows it — just flagging the tradeoff (macOS ships bash 3.2 by default, where namerefs aren't available, soevalmay be the pragmatic choice here). - No automated test coverage for the new flag-parsing/expansion logic (understandable for a bash dev-utility script, but a quick self-check invocation, e.g. a
--matrixdry run, in CI would catch regressions like the one above).
Verdict
Nothing here is a security or CI-breaking risk — it's an opt-in local dev tool with no production/CI wiring changed. The MATRIX_MODE arithmetic bug is real and demonstrable but narrow in scope. Flagging as should-fix-before-merge rather than a hard blocker — it's a quick one-line fix, and the documented usage patterns all work correctly as tested.
a5905e8 to
6462618
Compare
@rkennke thanks for the review; I've applied your suggestions in 6462618. |
Benchmark Results (commit afa13dd)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124061231 Commit: ✅ Within expected boundariesNo significant runtime deltas (all within run-to-run noise) and no internal-counter outliers. Runtime details (per benchmark × JDK)
Internal counter details (ddprof)ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
|
Benchmark Results (commit 9efcd6d)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124258722 Commit: ✅ Within expected boundariesNo significant runtime deltas (all within run-to-run noise) and no internal-counter outliers. Runtime details (per benchmark × JDK)
Internal counter details (ddprof)ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
|
rkennke
left a comment
There was a problem hiding this comment.
Looks good now. The codeql CI jobs should be fixed in the main branch, merging main into the PR branch should fix that.
Benchmark Results (commit afe7daf)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124304145 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10105 ms (21 iters) | ✅ 10422 ms (21 iters) | ≈ +3.1% (±11.1%) | — / — |
| akka-uct | 25 | ✅ 8847 ms (24 iters) | ✅ 8857 ms (24 iters) | ≈ +0.1% (±9.7%) | — / — |
| finagle-chirper | 21 | ✅ 6026 ms (33 iters) | ✅ 5960 ms (33 iters) | ≈ -1.1% (±25.3%) | |
| finagle-chirper | 25 | ✅ 5490 ms (36 iters) | ✅ 5476 ms (36 iters) | ≈ -0.3% (±24.1%) | |
| fj-kmeans | 21 | ✅ 2773 ms (68 iters) | ✅ 2727 ms (68 iters) | ≈ -1.7% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2816 ms (66 iters) | ✅ 2812 ms (66 iters) | ≈ -0.1% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2080 ms (89 iters) | ✅ 2078 ms (89 iters) | ≈ -0.1% (±2.7%) | — / — |
| future-genetic | 25 | ✅ 2008 ms (92 iters) | ✅ 2130 ms (87 iters) | 🔴 +6.1% | — / — |
| naive-bayes | 21 | ✅ 1322 ms (130 iters) | ✅ 1267 ms (135 iters) | ≈ -4.2% (±32%) | — / — |
| naive-bayes | 25 | ✅ 1018 ms (168 iters) | ✅ 1014 ms (168 iters) | ≈ -0.4% (±31.7%) | — / — |
| reactors | 21 | ✅ 15499 ms (15 iters) | ✅ 16710 ms (15 iters) | 🔴 +7.8% | — / — |
| reactors | 25 | ✅ 17956 ms (15 iters) | ✅ 18538 ms (15 iters) | ≈ +3.2% (±5.1%) | — / — |
Internal counter details (ddprof)
ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
| Benchmark | JDK | Dropped rec | Dropped jvmti | Dropped trace | Skipped WC | AGCT fail | Unwind fail |
|---|---|---|---|---|---|---|---|
| akka-uct | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 1873 / 1980 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 2169 / 2170 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 8250 / 8274 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 8755 / 8578 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 4 | 1301 / 1276 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1293 / 1279 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 6 | 2990 / 3007 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 2832 / 2870 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 5 | 3508 / 3478 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 3472 / 3463 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1531 / 1783 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 1855 / 1854 | ✅ / ✅ | ✅ / ✅ |
What does this PR do?:
This PR allows to run all the CI tests matrix locally in containers.
Motivation:
Run the full CI tests matrix locally. It comes out to be easier to run full testsuite with agents and is also useful in case of infrastructure flakiness.
Additional Notes:
How to test the change?:
For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.Unsure? Have a question? Request a review!